Fix visibility modifiers#713
Conversation
thunderbiscuit
force-pushed
the
fix/visibility
branch
3 times, most recently
from
91c613c to
33b64f4
Compare
thunderbiscuit
force-pushed
the
fix/visibility
branch
from
74bf645 to
03c7535
Compare
| /// | ||
| /// 1. Set the `nLockTime` for preventing fee sniping. Note: This will be ignored if you manually specify a | ||
| /// `nlocktime` using `TxBuilder::nlocktime`. | ||
| /// `nlocktime` using `TxBuilder::nlocktime`. |
There was a problem hiding this comment.
Is the extra spacing here and in a couple other spots in the diff for any specific reason?
There was a problem hiding this comment.
Yeah those were warnings from cargo's linter (the fmt tool). I can't find the commit right now but do you remember when we had to turn off the "fail" on the fmt CI workflow when warnings were issued, and only fail on fmt's failures? Some of the warnings were not in our power to fix (they were coming from uniffi's side), and so the CI was failing even though they were just warnings and we couldn't do anything about them.
This had the downside of now letting us merge stuff even if there is a warning by cargo's linter. In particular you probably won't know there is a warning if you don't run cargo fmt locally (or just check), which I sometimes forget to do. Not a big deal, but in this case I just thought I'd deal with the warning. The commit that did that is 78f38cf.
reez
left a comment
reez
left a comment
There was a problem hiding this comment.
ACK 03c7535
Thanks for this PR, for one I always love this sorts of changes that just tighten up the codebase in some really nice way.
It also helped me discover a bit more about "when exporting to uniffi, visibility modifiers were not respected anyway", so I think I understand now that while pub(crate) was technically correct from a Rust perspective, it could be misleading because uniffi was exposing these functions regardless.
So this PR makes the code more honest/explicit about its actual visibility in the bindings 👍
|
Yeah love it too. It sort of bothered me at first that it appeared inconsistent but I put it out of my mind for a while. The macros just brought me more into the Rust side of things lately and I'm finding little snags like that. |
2 participants
This PR fixes our incorrect use of visibility modifiers on some of our types. Note that when exporting to uniffi, visibility modifiers were not respected anyway? Not sure how that works or if it's my understanding of Rust that is lacking here, but a private or crate-private struct should not be available to outside users, yet definitely was.
In any case this PR makes public the types and methods we want to expose to users of the bindings explicit.
Changelog notice
No changelog.
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingBugfixes: